Custom promise type for managing global sshd configuration#128
Custom promise type for managing global sshd configuration#128larsewi wants to merge 2 commits intocfengine:masterfrom
Conversation
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
larsewi
force-pushed
the
sshd
branch
5 times, most recently
from
57e9aac to
8156d8c
Compare
|
Thanks for submitting a PR! Maybe @craigcomstock can review this? |
larsewi
force-pushed
the
sshd
branch
2 times, most recently
from
d57cd30 to
dbc704a
Compare
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
olehermanse
left a comment
olehermanse
left a comment
There was a problem hiding this comment.
I think it's good enough for an experimental / first iteration, but there's many things we could consider doing differently. Left some comments in the code.
| { | ||
| packages: | ||
| "openssh-server" | ||
| policy => "present"; |
There was a problem hiding this comment.
Formatting wrong according to style guide;
https://docs.cfengine.com/docs/master/examples/tutorials/policy-writing/policy-style/#style-summary
| @@ -0,0 +1,21 @@ | |||
| MIT License | |||
|
|
|||
| Copyright (c) 2025 Northern.tech | |||
There was a problem hiding this comment.
| Copyright (c) 2025 Northern.tech | |
| Copyright (c) 2026 Northern.tech |
| promise agent sshd | ||
| # @brief Define sshd promise type | ||
| { | ||
| path => "$(sys.workdir)/modules/promises/sshd.py"; |
There was a problem hiding this comment.
In some of the other promise types we have used a longer and more explicit filename, like sshd_promise_type.py and similar, to avoid potential conflicts with import sshd. I could see this being valuable here too.
| sshd: | ||
| "global" | ||
| PermitRootLogin => "no", | ||
| PasswordAuthentication => "no", | ||
| Port => "22", | ||
| AllowUsers => @(allowed_users); | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
README should mention what happens if the user enters multiple sshd promises.
| try: | ||
| from cfengine_module_library import PromiseModule, ValidationError, Result | ||
| except ImportError: | ||
| sys.path.append(os.path.join(os.path.dirname(__file__), "../../libraries/python")) | ||
| from cfengine_module_library import PromiseModule, ValidationError, Result |
There was a problem hiding this comment.
Should have a comment explaining why this is necessary.
| ## What the module manages internally | ||
| 1. **Include directive** — ensures the base `sshd_config` includes the drop-in directory (`sshd_config.d/`) as its first non-comment directive | ||
| 2. **Drop-in directory** — creates the drop-in directory if it doesn't exist | ||
| 3. **Drop-in file** — writes directives to `sshd_config.d/00-cfengine.conf` |
There was a problem hiding this comment.
If you use the label (promiser) in the filename, it would make it possible to have multiple sshd promises, right?
| def validate_promise( # pyright: ignore[reportImplicitOverride] | ||
| self, promiser: str, attributes: dict[str, object], metadata: dict[str, str] | ||
| ): | ||
| for attr, value in attributes.items(): | ||
| if not isinstance(value, (str, list)): | ||
| raise ValidationError(f"Attribute '{attr}' must be a string or slist") |
There was a problem hiding this comment.
I think we should do a better attempt at validating the promise - is it possible to get a list of all valid configuration options from sshd and/or write to a temporary file and use sshd to validate this file?
| An arbitrary human-readable label that appears in log messages and reports. | ||
| Since there is only one global sshd configuration, the promiser is not used to identify a resource. | ||
| Example: `"global sshd config"`. |
There was a problem hiding this comment.
Not necessarily wrong, but when reading this, my first thought was that the option (PermitRootLogin) should be the promisers. (config filename could be another option, but I understand the desire to abstract away the filenames.) Maybe you could elaborate a bit on this in the README, why this was chosen, what the pros/cons and limitations are.
| def normalize_directive(directive: str) -> str: | ||
| """Normalize a directive by removing trailing comments and replacing = with a space.""" | ||
| directive = strip_trailing_comment(directive) | ||
| # Normalize separator (= or whitespace) to a single space | ||
| directive = re.sub(r"\s*=\s*", " ", directive, count=1) | ||
| return directive.strip().lower() |
There was a problem hiding this comment.
Docstring could mention why / when this is used.
| # Step 6: Verify the effective config matches the desired attributes | ||
| if result != Result.NOT_KEPT: | ||
| result = update_result(result, self.verify_effective_config(attributes)) |
There was a problem hiding this comment.
I think we should do this at the beginning of evaluate_promise. If all the values are already correct, it's kept, no need to set them again / twice?
3 participants
No description provided.